Remove ineffective rule #139
Conversation
I believe this rule is not achieving its intended result. If I understand the intention of the rule md5 should be avoided where collisions can be generated to cause a lapse in security. However in those cases the suggested approach to replace md5 with sha256 I would still consider bad advice. If you are implementing security one should not roll your own.
To judge the effectiveness I ran the following on the code base:
```
git checkout 2.2.0
grep -r --exclude-dir='Test' 'md5(' app/code lib/internal | wc -l
52
git checkout 2.3-develop
grep -r --exclude-dir='Test' 'md5(' app/code lib/internal | wc -l
53
grep -r --exclude-dir='Test' 'phpcs:ignore Magento2.Security.InsecureFunction' app/code lib/internal | wc -l
4
```
In summary we have added more exceptions to the rule than made changes after the rule was introduced.
I do not believe that removing md5 is warranted in a lot of cases (the exceptions allowed confirm this) and that just removing md5 improves security by itself (in other words `hash('string', 'sha256')` is only marginally better than `md5('string') where really sensitive data is involved, as no one should be handling this directly in the first place.
|
|
|
@piotrekkaminski would be great to have your input here. |
|
To add a recent example of a desired change the current rule and suggestion would have resulted in something like |
|
Seems like Piotr won't share his opinion on this one :) @lenaorobei are there some details you could share with us regarding putting on hold? |
|
@orlangur this requirement came from the security team and put on hold since there were non resolved concerns. If possible please create an issue for the future discussion. |
|
Would it still be possible to discuss the effectiveness of this rule? I've just scanned the In my opinion, it is best to update the rule. Indeed, |
…standard into fooman-patch-1
|
@fooman thank you for the pull request. We cannot remove the md5 check from the tests, but you are right about the replacement recommendation, it can be improved. The check for md5 can be skipped for lines where md5 is used intentionally and there is a good reason why it's preferred over other algorithms. However, a static test should report this issue to ensure it is discussed during code review. Existing usages of md5 may require review and update. |
Fix phpcs issues
I believe this rule is not achieving its intended result. If I understand the intention of the rule md5 should be avoided where collisions can be generated to cause a lapse in security. However in those cases the suggested approach to replace md5 with sha256 I would still consider bad advice. If you are implementing security one should not roll your own.
To judge the effectiveness I ran the following on the code base:
In summary we have added more exceptions to the rule than made changes after the rule was introduced.
I do not believe that removing md5 is warranted in a lot of cases (the exceptions allowed confirm this) and that just removing md5 improves security by itself (in other words
hash('string', 'sha256')is only marginally better thanmd5('string')where really sensitive data is involved, as no one should be handling this directly in the first place.